refactor(table): carry the root caret offset in a WeakMap, not a DOM stamp - #359
Merged
Conversation
…stamp TableBlockWidget's margin-click caret fallback read the block start back out of `data-doc-from`. That channel existed only because `updateDOM` reuses the root element across widget instances and cannot re-bind the click listener, whose captured `this` stays the old instance — the same problem the image widget had until #356. The channel does not have to be the DOM. A module-private `WeakMap<HTMLElement, number>` has the same per-element, updateDOM-writable lifetime — the pattern `pendingDrag` already uses two declarations above — and keeps the value a number end to end, so there is no malformed state left to gate. Both block widgets now share one root channel. The per-CELL stamps stay on the DOM: `cellPointAt` resolves an arbitrary descendant under the pointer and no closure knows which cell was clicked, so `stampedOffset` keeps gating those reads. `data-doc-from` is still written for DOM inspection but never read back. This also removes the chain's weakest arm. The old third arm — the toDOM-time `this.docFrom` closure, reachable once both stamps failed the gate — could reveal a DIFFERENT block after a positional shift. The block start now always comes from a channel `updateDOM` re-points, so the "a degraded caret still reveals this table" guarantee holds unconditionally; the remaining fallback is unreachable by construction and logs if it is ever hit. Behaviour is unchanged in every reachable state — nothing outside Quoll writes that attribute — so this buys an unrepresentable failure state, not a fix.
… comment - Add a test that pins the blockStart WeakMap write in toDOM: existing margin-click tests couldn't kill that deletion because the miss fallback (widget.docFrom) dispatches the same anchor value in every fixture here. The new test asserts the miss console.error breadcrumb is NOT called, which the deletion does trigger. - Correct the kill-matrix comment above the data-doc-from it.each block: it previously claimed both rows redden on any attribute-read revert, but the gated pre-refactor read (stampedOffset ?? this.docFrom) only reddens the well-formed-but-wrong row — the malformed row falls through that gate to the same value the test expects. Verified both revert flavours by temporarily restoring each expression and running the suite.
…tating it Comment-only pass over this PR's diff. No executable code, test rows, or assertions changed. The "checkSelection only rejects range.to > doc.length, so NaN lands a silently broken selection" argument was being re-derived three times here. It already lives on `stampedOffset` in cell-point.ts, whose docblock names this file's caret path as a consumer, so `blockStart` and the click listener now point there and keep only their own local claims: why the block start needs no gate, why the cell offset must stay on the DOM and go through one, and why the "degraded caret still reveals this table" guarantee is now unconditional. The two test comments lose their length without losing a fact — both kill matrices and the non-vacuity argument survive verbatim in substance — and drop two self-references to notes that exist nowhere in the repo.
- Fix the 'both rows are needed' overstatement on the data-doc-from kill-matrix comment: '999' alone kills both reverts; 'abc' is redundant for kill power there but isolates a separate NaN-silent failure mode against the bare read. - Narrow the blockStart margin-click comment's universal claim: only fixtures that click right after toDOM share docFrom between the WeakMap entry and the fallback closure. The updateDOM re-stamp fixture stays green for an unrelated reason (its own blockStart.set write re-fills the entry).
3 tasks
mtskf
added a commit
that referenced
this pull request
Aug 16, 2026
…umb click (#360) Every click fixture in cm-image-widget.test.ts builds the widget and clicks it in the same breath, so the listener's miss fallback (this.docFrom, the toDOM-time closure) coincides with the WeakMap value: deleting blockStart.set(root, this.docFrom) from toDOM left the whole suite green. Assert instead that no miss breadcrumb is logged for a freshly-built widget, which is the only observable difference. Mirrors the pin PR #359 added on the table widget, closing the same gap on the image widget that introduced the pattern.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Converges
TableBlockWidget's ROOT position channel on the same module-privateWeakMapthe image widget adopted in #356, so both block widgets carry their block start the same way. The per-CELL stamps deliberately stay on the DOM.Changes
table-widget.ts: new module-privateblockStart: WeakMap<HTMLElement, number>(alongside the existingpendingDrag, same rationale), written intoDOMbeside the listener and re-pointed inupdateDOM. The click listener's caret fallback now reads that map instead ofstampedOffset(root, "data-doc-from").data-doc-fromis still written for DOM inspection but never read back.this.docFromclosure — is gone as a routine path. It was the one link that could reveal a different block after a positional shift, sinceupdateDOMcannot re-bind the listener. The remainingblockStartmiss is unreachable by construction and logs an invariant-violation breadcrumb (identifying the widget byslice) before falling back, mirroringimage-widget.ts.data-cell-from/data-cell-tostay on the DOM and keep going throughstampedOffset:cellPointAtresolves an arbitrary descendant under the pointer, so no closure knows which cell was clicked.updateDOMre-stamp).Behaviour is unchanged in every reachable state — nothing outside Quoll writes that attribute — so this removes an unrepresentable failure state rather than fixing an incident.
Related
WeakMapthe image widget now uses" (deferred from fix(image): carry the caret offset in a WeakMap, not a DOM stamp #356)Test Plan
test/webview/table/cm-table-widget.test.ts: the malformed-root-stamp test is transposed to the new channel — now anit.eachasserting a"abc"and a well-formed-but-wrong"999"data-doc-fromwritten onto the root are both ignored (the "999" row is what a format gate alone would have missed).blockStart.set(dom, …)inupdateDOMreddens "re-stamps offsets on updateDOM so a click after a shift uses the new base" (anchor 0 instead of 5); reverting the fallback tostampedOffset(root, …)reddens the "999" row; reverting it to a bareNumber(root.dataset.docFrom)reddens both rows.pnpm compile,pnpm lint,pnpm test:unit(4974 passed),pnpm test:browser(36 passed).pnpm build+pnpm package(vsix audit clean) +code --install-extension --force.